Repository navigation
Reuse the objective constraint and order media bound assignment - #166
Merged
Fxe merged 1 commit intoNov 11, 2026
Merged
Conversation
Two independent fixes in constraint and bound construction, each with a test that fails without it. 1. ObjConstPkg added a NEW constraint on every build_package call. Calling it twice -- which gapfilling does when it tightens an objective threshold -- left two objc constraints on the solver, the stricter one silently binding. build_constraint now compares the linear coefficients of each existing objc constraint against the current objective and, on a match, updates that constraint's bounds in place and returns it. build_package also now RETURNS the constraint; previously it discarded it, so a caller had no handle to adjust later. 2. KBaseMediaPkg assigned exchange bounds lower-then-upper unconditionally. When the media's uptake implies a lower bound ABOVE the reaction's current upper bound -- a reaction already restricted to a negative upper bound -- that assignment order passes through a state with lb > ub. The upper bound is now raised first in that case. KBaseMediaPkg also records the applied media on self.current_media, so later packages can see what media is in force instead of re-deriving it. Tests: tests/fbapkg/test_objconst_and_media_bounds.py, 5 cases on an inline two-reaction model -- constraint reuse across repeated builds, the returned handle, a genuinely different objective still getting its own constraint, the inverted-bound ordering case, and current_media initialisation. Verified on both sides: 3 of the 5 fail against the unpatched tree, and the bound-ordering case was corrected after an initial version passed either way and therefore guarded nothing. Full suite: 63 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this accomplishes
Two independent fixes in constraint and bound construction. Each ships a test that fails without it.
1.
ObjConstPkgaccumulated duplicate objective constraintsbuild_packageadded a new constraint on every call. Calling it twice — which gapfilling does when it tightens an objective threshold — left twoobjcconstraints on the solver, with the stricter one silently binding. Nothing errored; the model was just more constrained than the caller asked for.build_constraintnow compares the linear coefficients of each existingobjcconstraint against the current objective and, on a match, updates that constraint's bounds in place and returns it. A genuinely different objective still gets its own constraint.build_packagealso now returns the constraint. Previously it discarded the return value, so a caller had no handle to adjust the bounds later.2.
KBaseMediaPkgcould pass through an inverted bound stateExchange bounds were assigned lower-then-upper unconditionally:
When the media's uptake implies a lower bound above the reaction's current upper bound — a reaction already restricted to a negative upper bound — that order passes through a state where
lb > ub. The upper bound is now raised first in that case.KBaseMediaPkgalso records the applied media onself.current_media, so packages built later can see what media is in force rather than re-deriving it.Tests
tests/fbapkg/test_objconst_and_media_bounds.py— 5 cases on an inline two-reaction model: constraint reuse across repeated builds, the returned handle, a genuinely different objective still getting its own constraint, the inverted-bound ordering case, andcurrent_mediainitialisation.Verified on both sides: 3 of the 5 fail against the unpatched tree. The bound-ordering case was corrected after an initial version passed either way and therefore guarded nothing — it now sets
upper_bound = -200against a media uptake of 100, which is the condition the reorder exists for.No fixture files, so these tests need no biochemistry database or commercial solver licence.